Skip to content

do not cut off bottom portion of text #2568

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

dajrivera
Copy link

@dajrivera dajrivera commented Jul 15, 2025

Summary

The bottom portion of texts get cut off on Edge, Chrome, & Firefox; this issue does not appear in Safari. Using CSS property line-height: normal seems to fix the issue and does not affect Safari's rendering of the page.

before:
image

after:
image

Related issue, if any:

N/A

What kind of change does this PR introduce?

Other: minor UI fix

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed

Does this PR introduce a breaking change?

No

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge

Copy link

vercel bot commented Jul 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docsify-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 24, 2025 3:04am

@sy-records
Copy link
Member

Thanks for your contribution.

But I think this is by design, and you can modify it through the theme properties.

      :root {
        --heading-line-height: normal;
      }

@paulhibbitts
Copy link
Collaborator

paulhibbitts commented Jul 16, 2025

I'd tend to include this fix, as by default I would expect line-height to be automatic by default in all Browsers and not require additional changes.

@dajrivera
Copy link
Author

@sy-records ah, thanks; I did not check for an existing variable. I set the global variable and it did indeed fix the problem but as @paulhibbitts commented, I would expect the text to look correct by default in all browsers. I've moved the change to be the default --heading-line-height variable value so it can still be overridden by themes.

@paulhibbitts
Copy link
Collaborator

Thanks @dajrivera , now that I think about it I may also have run across this issue earlier when trying different fonts out with v5 and noticing I needed to then further adjust line-height to avoid slight cut-off one some particular ones.

paulhibbitts
paulhibbitts previously approved these changes Jul 17, 2025
Copy link
Collaborator

@paulhibbitts paulhibbitts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sy-records
Copy link
Member

I think there might be a problem.

image

* calculation must use units; 'normal' value breaks calculation
* replace --header-line-height variable with line height unit (lh)
* introduce line heigh multiplier to reduce or increase line height
@dajrivera
Copy link
Author

dajrivera commented Jul 17, 2025

@sy-records looks like the h2 bottom-margin is calculated and was the reason for the prior "Unit required" comment; using value normal here broke that calculation. I've updated the bottom-margin calculation using the line height unit instead. At this point the bottom margin was too large so I included a local line-height-multiplier to reduce the margin size as a workaround. Let me know if you can think of a better alternative or a more appropriate method to implement this.

Looks good to me in Edge. I can test Chrome, Firefox, and Safari tomorrow.

@paulhibbitts
Copy link
Collaborator

paulhibbitts commented Jul 17, 2025

Good catch @sy-records , my apologies for not viewing additional pages in the PR preview... I will be more careful in the future.

From my perspective I hope that the resulting spacing unit adjustments match the spacing as originally specified by John Hildenbiddle as much as possible.

* change h2 margin calculation to use lh and em units to allow
    heading-line-height var to be any valid value (no unit requirement)
@dajrivera
Copy link
Author

dajrivera commented Jul 23, 2025

@sy-records @paulhibbitts Please review this again. I've changed the approach to only allow y overflow on h1 & h2 headings. Tested this on Edge, Chrome, Firefox, and Safari and all display the fonts without issue without additional changes to the rest of the UI.

If OK, let me know I can clean up the commits.

@paulhibbitts
Copy link
Collaborator

Thanks @dajrivera , but it looks like it strays quite a bit from the original theme spacing, re: H2 border and spacing.

Here is the current Docsify Preview:
2025-07-23_14-50-09

And here is the latest PR Preview I see:
2025-07-23_14-50-00

@dajrivera
Copy link
Author

@paulhibbitts sorry about that, seems the calc function went missing when reverting a change that was affecting safari; I've re-added it again.

@paulhibbitts
Copy link
Collaborator

No worries, now the preview looks identical to John's original layout ✅

Here is the current Docsify Preview:
2025-07-23_15-16-07

And here is the updated PR Preview:
2025-07-23_15-16-21

All looks good to me!

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a text clipping issue where the bottom portion of text gets cut off in Edge, Chrome, and Firefox browsers. The fix involves modifying the overflow CSS properties to prevent vertical text clipping while maintaining horizontal overflow control.

  • Replaces single overflow: hidden property with separate overflow-x and overflow-y properties
  • Sets overflow-x: clip to handle horizontal overflow and overflow-y: visible to prevent vertical text clipping

@@ -251,7 +251,8 @@

/* Prevent long titles from causing horizontal scrolling */
&[id] a {
overflow: hidden;
overflow-x: clip;
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overflow-x: clip property has limited browser support compared to overflow-x: hidden. Consider using overflow-x: hidden for better browser compatibility, as clip may not be supported in older browsers.

Suggested change
overflow-x: clip;
overflow-x: hidden;

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants